-
Notifications
You must be signed in to change notification settings - Fork 187
Add URL validation to operational server webhooks #1887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
The URL validation part seems fine to me.
Regarding URL sanitization, see inline comments.
Regarding the error handling,
When operational webhooks fail to deliver with a 404 error, the system logs a generic "Operational webhooks are enabled but no listener set for {org_id}" message, without distinguishing between different failure scenarios (e.g., incorrect URL format, missing application, or missing endpoint).
This does not make sense to me. Incorrect URL format is something we should be catching earlier, so no need to try to distinguish this when receiving a 404. No endpoints for the app does not result in an error at all.
Thank you @svix-jplatte for all you helps |
Motivation
This PR addresses issue #1138 "Handling of operational webhooks server name" which was identified after users experienced problems configuring and troubleshooting operational webhooks in local development environments.
The current implementation of operational webhooks has two key issues:
The server URL provided in
SVIX_OPERATIONAL_WEBHOOK_ADDRESS
is not validated or sanitized before use, leading to problems when users provide URLs with trailing slashes or incorrect formats.When operational webhooks fail to deliver with a 404 error, the system logs a generic "Operational webhooks are enabled but no listener set for {org_id}" message, without distinguishing between different failure scenarios (e.g., incorrect URL format, missing application, or missing endpoint).
These issues make it difficult to diagnose and resolve operational webhook configuration problems, as referenced in discussion #1125 where a user couldn't get operational webhooks working locally.
Solution
This PR implements three key improvements to the operational webhooks handling:
URL Validation: Added validation for the
operational_webhook_address
configuration value incfg.rs
that verifies:URL Sanitization: Enhanced the
OperationalWebhookSenderInner::new
method inoperational_webhooks.rs
to properly sanitize URLs by:Improved Error Handling: Expanded the error handling in
send_operational_webhook
to provide more detailed diagnostics: